-
-
Notifications
You must be signed in to change notification settings - Fork 42
chore: Attempt to cache source from downloads.keyman if external source missing #337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Attempt to cache source from downloads.keyman if external source missing #337
Conversation
If external source is unavailable, get the latest version from downloads.keyman
mcdurdin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good interim stop-gap. I am concerned that we will not see the warnings in the logs which may lead to bigger issues in the future, so we need to have a tracking issue for a deeper resolution (which involves caching the files in-repo at submission time). Perhaps this patch should not 'fix' the reporting issue
resources/external.inc.sh
Outdated
| local path= | ||
| [[ ! $filename =~ .model_info$ ]] && path=source/ | ||
| curl -s -L "$url" --output "$path$filename" --create-dirs || die "Unable to download $filename" | ||
| curl -s -L "$url" --output "$path$filename" --create-dirs || retrieve_cached_model "$path" "$filename" && continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't continue if retrieve_cached_model fails; we still also need to verify the checksum of the cached model.
resources/external.inc.sh
Outdated
| # Also handle model_info since we need to query the latest version | ||
| # .model_info is downloaded up a level | ||
| local version=`curl "$query" | $JQ -r '.[].version'` | ||
| local model_info_filename="https://downloads.keyman.com/models/${model_id}/${version}/${model_id}.model_info" | ||
| curl -s -L "$model_info_filename" --output "$model_id.model_info" --create-dirs || die "Unable to download $model_info_filename" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should really be under the model_info) section, I don't see any benefit to it being under kmp)
* Remove continue, process model_info in its own section
|
Relates-to: keymanapp/keyman#15545 |
mcdurdin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; we'll keep the tracking issue in keymanapp/keyman open for a longer-term solution that involves caching on first use
The current build is failing because the external sources for fv.bea.tsaadane are not available
Changes
I'm open to optimizing the api.keyman query so we only run it once.
Also am not handling
retrieve_external_source_model()at the moment.Test-bot: skip